Skip to content

[APP-16062] Reland Remove-Reconfigure + cascade fix consolidation#6098

Open
danielbotros wants to merge 3 commits into
viamrobotics:mainfrom
danielbotros:app-16062-reland-remove-reconfigure
Open

[APP-16062] Reland Remove-Reconfigure + cascade fix consolidation#6098
danielbotros wants to merge 3 commits into
viamrobotics:mainfrom
danielbotros:app-16062-reland-remove-reconfigure

Conversation

@danielbotros

@danielbotros danielbotros commented Jun 9, 2026

Copy link
Copy Markdown
Member

Summary

Relands Remove-Reconfigure after #5997

Commits

1. Revert of #5997 (8e54770cf)

Restores the post-#5944 state — Remove-Reconfigure + cascade fix together — by reverting the previous PR. The git revert produced merge conflicts in four files due to drift on main during the bake window (primarily from #6041, RSDK-13417, "skip weak/optional dep reconfigure when unchanged"). Resolved by preserving #6041's snapshot-tracking + skip-when-unchanged optimization adapted to Remove-Reconfigure semantics.

Main conflicts:

robot/impl/resource_manager.go — 3 conflict regions:

  • Remote-resource discovery (around line 305): nodeAlreadyExists branching. HEAD had an inverted if !nodeAlreadyExists condition with an unconditional SwapResource later in the function; parent had if nodeAlreadyExists { SwapResource } else { ... }. Took HEAD's structure — its end-of-block SwapResource advances the clock for both new and existing nodes, which snapshot tracking depends on. Parent only advanced for existing. Side fix: parent's graph_node.go adds an incrementClock bool parameter to SwapResource, so the post-merge call site needed , true appended.

  • processResource dependency fetch (around line 1125). HEAD used getDependenciesWithWeakOptionalSnapshot; parent used the simpler getDependencies. Initially merged hybrid (snapshot variant, discard deps), later reverted to parent verbatim (_, err := lr.getDependencies(...)) — see the fix in commit 3.

  • processResource rebuild path (lines 1140-1167): in-place reconfigure vs. always-rebuild. HEAD had a big gNode.ResourceModel() == conf.Model block calling moduleManager.ReconfigureResource or currentRes.Reconfigure with MustRebuildError fallback, returning (resource.Resource, bool, error). Parent had no in-place path and always rebuilt, returning (resource.Resource, error). Took parent entirely — Remove-Reconfigure removes the public Reconfigure method and moduleManager.ReconfigureResource, so the in-place branch can't exist. Function signature is back to 2-value; caller's markChildrenForUpdate becomes unconditional.

robot/impl/local_robot.go — 1 conflict region:

  • updateWeakAndOptionalDependents action (around line 1180). HEAD had skip-when-unchanged guard (weakOptionalDepClocksEqual) plus an in-place Reconfigure. Parent had no skip guard and a BuiltInReconfigure for internal resources and inline rebuild for any other resource. Hybrid: kept HEAD's skip guard + "handling weak/optional update" log and took parent's BuiltInReconfigure / inline-rebuild branch.

Test-file conflicts in robot/impl/optional_dependencies_test.go and robot/impl/robot_reconfigure_weak_dependencies_test.go are not enumerated here (many); they were resolved by resetting both test files to parent and selectively restoring the new tests/helpers #6041 added.

2. Cleanup (d73416078)

I noticed some rebuild/cascade logic duplication within rebuildResource, cascadeRebuildDependentsOf, and ReconfigureResource so I took the chance to clean it up. I merged rebuildResource and rebuildResourceWithVisited into a single rebuildResource that takes a visited parameter (nil from outer callers, threaded from recursive ones), and replace the inline cascade walk inside the rebuild path with a call to cascadeRebuildDependentsOf (now also taking a visited parameter). Both the addResource post-add hook and the rebuildResource post-rebuild step now go through the same cascade implementation.

3. Post-merge audit (257182945)

A combing pass over the merge product to fix problems my initial conflict resolution introduced:

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jun 9, 2026
@danielbotros danielbotros marked this pull request as draft June 9, 2026 18:48
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jun 10, 2026
@danielbotros danielbotros force-pushed the app-16062-reland-remove-reconfigure branch from 2571829 to 72050f3 Compare June 10, 2026 19:55
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jun 10, 2026
@danielbotros danielbotros marked this pull request as ready for review June 10, 2026 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test This pull request is marked safe to test from a trusted zone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants